Skip to content

add a new functions - Remove#172

Closed
Luoxin wants to merge 6 commits intoelliotchance:masterfrom
Luoxin:master
Closed

add a new functions - Remove#172
Luoxin wants to merge 6 commits intoelliotchance:masterfrom
Luoxin:master

Conversation

@Luoxin
Copy link
Copy Markdown
Contributor

@Luoxin Luoxin commented Sep 6, 2021

#171


This change is Reviewable

@Luoxin
Copy link
Copy Markdown
Contributor Author

Luoxin commented Sep 6, 2021

#171

Comment thread functions/main.go Outdated
{"Unique", "unique.go", ForNumbersAndStrings, "n"},
{"Unshift", "unshift.go", ForAll, "n"},
{"Values", "values.go", ForMaps, "n"},
{"Remove", "remove.go", ForNumbersAndStrings, "n⋅n"},
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep these lines sorted by name.

Also, O(n^2) isn't quite correct, it would be O(kn) but more on that below.

Comment thread functions/remove.go
Comment thread functions/remove.go
Comment thread functions/remove.go Outdated
@@ -0,0 +1,19 @@
package functions

// Remove items from slice when item existed
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Remove returns a new slice that does not include any of the items.

@elliotchance
Copy link
Copy Markdown
Owner

@Luoxin thanks for creating this diff, with a few tweaks I'm happy to merge. You will also need to update the README.

- change n^2 to 2n
@Luoxin
Copy link
Copy Markdown
Contributor Author

Luoxin commented Sep 7, 2021

Thank you for the correction, I made some changes

Comment thread README.md Outdated
| [`Unique`](#unique) | ✓ | ✓ | | | n | Unique returns a new slice with all of the unique values. |
| [`Unshift`](#unshift) | ✓ | ✓ | ✓ | | n | Unshift adds one or more elements to the beginning of the slice and returns the new slice. |
| [`Values`](#values) | | | | ✓ | n | Values returns the values in the map. |
| [`SortUsing`](#sortusing) | ✓ | | ✓ | | n⋅log(n) | SortUsing works similar to sort.Slice. However,
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format of the table has been mangled. Please fix.

Comment thread README.md Outdated

Due to Go's randomization of iterating maps the order is not deterministic.

## Remove
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep the titles sorted.

Comment thread functions/main.go Outdated
{"Unshift", "unshift.go", ForAll, "n"},
{"Values", "values.go", ForMaps, "n"},
{"Remove", "remove.go", ForNumbersAndStrings, "n⋅n"},
{"Remove", "remove.go", ForNumbersAndStrings, "2n"},
Copy link
Copy Markdown
Owner

@elliotchance elliotchance Sep 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O(2n) is not a valid complexity, the constant would be removed to O(n). Also, you must keep the functions sorted.

- readme
Comment thread README.md
the slice and returns the new slice. | | [`Values`](#values) | | | | ✓ | n | Values returns the
values in the map. | | [`Remove`](#values) | ✓ | ✓ | | ✓ | 2n | Remove returns a new slice that does
not include any of the items. |
| [`SortUsing`](#sortusing) | ✓ | | ✓ | | n⋅log(n) | SortUsing works similar to sort.Slice. However, unlike sort.Slice the slice returned will be reallocated as to not modify the input slice. |
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still mangled, you can repair this by restoring the original file:

git checkout origin/master -- README.md

Now add the Remove line back in and commit again.

Comment thread README.md
| [`Product`](#product) | | ✓ | | | n | Product is the product of all of the elements. |
| [`Random`](#random) | ✓ | ✓ | ✓ | | 1 | Random returns a random element by your rand.Source, or zero |
| [`Reduce`](#reduce) | ✓ | ✓ | | | n | Reduce continually applies the provided function over the slice. Reducing the elements to a single value. |
| [`Remove`](#remove) | ✓ | ✓ | | ✓ | n | Remove returns a new slice that does not include any of the items. |
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maps should not be ticked.

Did you format this file after running generate.go?

@elliotchance
Copy link
Copy Markdown
Owner

@Luoxin - if you want to revisit this, you can build against v2 which is much more flexible.

@Luoxin
Copy link
Copy Markdown
Contributor Author

Luoxin commented Mar 24, 2022

I will do something in this weekend

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants